🦙 MegaLinter status: ✅ SUCCESS

See detailed report in MegaLinter reports

MegaLinter is graciously provided by

Please could Release Management kindly review this PR to allow merge to main / release with tag r1.1

Thanks @Kevsy ... I have created camaraproject/ReleaseManagement#74 for the review which I will take next week.

I see that the CHANGELOG is already partly updated in advance, and also the version within in the YAML files are already set (should have happen in the release PR, but ok).

Few points which you can already address here in the PR:

• the update of the README within the section Release Information (it has to be done in the Release PR as it is otherwise not part of the release tag)
• add in the Changelog the section for the application-profile API
• add the links to the .feature files in the API Readiness Checklist

Have a nice weekend!

Thanks @hdamker , changes to address your points now committed.

@Kevsy please delete the tag r1.1 which you created four days ago. It points to the merge commit of #66 which is NOT the approved release candidate. The correct process is described step by step here: https://github.com/camaraproject/ReleaseManagement/blob/main/documentation/API_Release_Guidelines.md#releasing-an-api-step-by-step.

Please find my review comments on connectivity-insights.yaml:

There are some critical deviations from the API Design Guidelines:

• /network-quality/insights:
  • I don't understand the use of the of path element network-quality ... it would need to be a resource, but it isn't one.
    • If you think that this API is about network quality insights, why not calling the API network-quality-insights and the endpoint just /check or similar (it must be a verb, see below)?
    • if you are fine with the API name connectivity-insights, why not just use a verb here for this endpoint? check-network-quality might be an option, avoiding the second level
  • the operation is actually not creating a resource, the description is mentioning an insightId but that is not returned. If it would create a resource, there must be read and delete operations in the API.
  • it seems to be a "GET insights" using a POST operation due to the sensible information in the body, in which case the following guideline applies (cf. https://github.com/camaraproject/Commonalities/blob/r0.4.0-rc.2/documentation/API-design-guidelines.md#post-or-get-for-transferring-sensitive-or-complex-data):

When the POST method is used, the resource in the path must be a verb (e.g. retrieve-location and not location) to differentiate from an actual resource creation.

• subscriptions endpoints don't follow the guidelines in https://github.com/camaraproject/Commonalities/blob/r0.4.0-rc.2/documentation/API-design-guidelines.md#resource-based-explicit-subscription
  • it must be a separate API with -subscriptions in the name, the endpoint would be just /subscriptions, the API could connectivity-insights-subscriptions,

To ensure consistency across Camara subprojects, it is necessary that explicit subscriptions are handled within separate API/s. It is mandatory to append the keyword "subscriptions" at the end of the API name. For e.g. device-roaming-subscriptions.yaml

• subscription events are not consistent between the examples in line 864-865 and 976.
• The schema NetworkQualityTypeEvent seems to be incomplete, it has no properties:

    NetworkQualityTypeEvent:
      description: event structure for event-type event
      allOf:
        - $ref: "#/components/schemas/CloudEvent"
        - type: object

Further less critical comments, but need to addressed:

• scope name for the network quality insight connectivity_insights:network_quality_insight:write does not follow https://github.com/camaraproject/Commonalities/blob/r0.4.0-rc.2/documentation/API-design-guidelines.md#1161-scope-naming, with defines the structure as api-name:[resource:]action

  • the api-name is connectivity-insights, not connectivity_insights
  • there is no resource involved, see above
  • For POST operations with a verb, the action will be a verb, not a CRUD operation.

• info.description - Authorization and Authentication: replace lines 62+63 with the updated text from ICM r0.2.0-rc.2 (link to main replaced). This can be done here in the PR or as an issue after the r1.1.

• info.version: 0.4.0-rc.1 instead of "0.4.0-rc.1" (cf API-design-guidelines.md#info-object)

• info.contact is optional and and may be added by API Providers documenting their APIs

• responses:

  • CreateSubscriptionBadRequest400: "OUT_OF_RANGE" missing
  • Generic400: not aligned with https://github.com/camaraproject/Commonalities/blob/r0.4.0-rc.2/artifacts/CAMARA_common.yaml, "OUT_OF_RANGE" missing
  • CreateSubscription403: not aligned with https://github.com/camaraproject/Commonalities/blob/r0.4.0-rc.2/artifacts/camara-cloudevents/event-subscription-template.yaml, e.g. "INVALID_TOKEN_CONTEXT" missing, typo in line 1143 "SUSBCRIPTION_MISMATCH"
  • Device related error responses as defined in https://github.com/camaraproject/Commonalities/blob/r0.4.0-rc.2/documentation/API-design-guidelines.md#62-error-responses---device-object seem to be missing, e.g. in 422.
  • ... please check all error responses for alignment

@Kevsy @maheshc01 Please see my comments above - I've stopped after the review of the connectivity-insights.yaml. How do you want to proceed? One option could be to first finish the connectivity-insights with one endpoint, and postpone the subscription API to a later release.

@hdamker Many thanks for the thorough review, and fully noted. Release tag has been deleted. Regarding the YAML we will confirm following the COI call later today.

Below my preferences based on the helpful advice above,:

if you are fine with the API name connectivity-insights, why not just use a verb here for this endpoint? check-network-quality might be an option, avoiding the second level

• my preference /connectivity-insights/{version}/check-network-quality

the operation is actually not creating a resource, the description is mentioning an insightId but that is not returned.

• I think we should not create a resource, rather the following point applies here:

it seems to be a "GET insights" using a POST operation due to the sensible information in the body, in which case the following guideline applies

• ...hence we should refactor according to the guideline

So those are the critical points for connectivity-insights.yaml , and the others will be addressed too.

One option could be to first finish the connectivity-insights with one endpoint, and postpone the subscription API to a later release.

@mahesh, we can deicide this on the call later

@Kevsy thanks for the response. Regarding application-profiles there are also some comments and questions, but less severe. Should I add them here in similar way as above or would you prefer to have them in an issue?

Should I add them here in similar way as above or would you prefer to have them in an issue?

Thanks @hdamker - please add them here for now, and then we can break them all out in separate issues to make sure they are all covered. We can do this after the call today.

Please find my review comments on application-profiles.yaml:

• L9 - info.description: the description is quite short and less concrete then the tag description in line 28/29. Might make sense to mention that there are future extensions planned. The mandatory text about Authorization and Authentication from ICM is missing
• L36 - scope naming: connectivity_insights:policy:write -> application-profiles:write (resource is optional according to https://github.com/camaraproject/Commonalities/blob/main/documentation/API-design-guidelines.md#1161-scope-naming, otherwise it would be application-profiles:application-profiles:write)
• L48 - Why "*/*"? The API Design Guidelines mandate application/json (comment applies also to connectivity-insights.yaml)
• L50 - Consider to define a schema like CreateApplicationProfile which contains today only NetworkQualityThresholds but can be extended with further optional fields (see also below).
• L53 - response should be 201 (Created), not 200
• L58 - Why returning only the ApplicationProfileId and not the full representation of the resource object? (that is inconsistent with updateApplicationProfile where the complete resource object is returned)
• L65 - 404 is not an expected response, as the resource will be just created
• L67 - 409 - which kind of "CONFLICT" do you expect here? How would a test case look like to generate this error message ?
• L75 - Consider "PUT" vs "PATCH".
  • "PUT" will replace the complete resource object. It would also require some more definition what happens if
    • a) the applicationProfileId in the path does not yet exist (PUT would allow to create it, but I don't think that's what you want here, as the ids are generated on the server side) and
    • b) if the applicationProfileId in the path and the applicationProfileIdin the requestBody do not match (that should be an error response)
  • both topics can be avoided by using "PATCH" and in the requestBody NetworkQualityThresholds (or some UpdateApplicationProfile structure). That would be consistent with createProfile and with the description: "Update the network quality thresholds for an application ...". In this case you need to define that the NetworkQualityThresholds are replaced as a whole.
• L78 - scope name: connectivity_insights:application_profile:write -> application-profile:write (cf. L36)
• L82 - "Update the network quality thresholds for an application ..." -> "Update the network quality thresholds of an application profile ..." (the profile can be used by multiple applications). Also the semantic of the PUT or PATCH should be defined, see comment on L75
• L88 - typo
• L97 - this description does not fit to "PUT" as PUT updates the entire resource with data that is passed in the body payload.
• L99 - see comment on line 48
• L129 - scope name, see above
• L159 - see comment on L67
• L169 - scope name, see above
• L195 - see comment on L67
• L313 ApplicationProfile definition:
  • shouldn't the applicationProfileId be required?
  • would an (optional) name (freely chosen by the API user) make sense?
• L358ff - responses: consider to copy the used responses from https://github.com/camaraproject/Commonalities/blob/main/artifacts/CAMARA_common.yaml for consistency (but most responses do not apply for this API)
  • L359 in Generic400 ... "OUT_OF_RANGE" is missing from CAMARA_common.yaml

hi @rartych ,
thank you for the review comments. Have accepted all of your commit suggestions and commits additional changes against issues created based on your review comments.
Please take a closer look at issue #95 . Need your inputs on that in case i missed something.

Thank you!

Hi @Kevsy i see @rartych has approved the PR. Could you approve the changes, merge and advice on the next step of release mgmt.

@Kevsy will you also create the r1.1 release or should I take that for you?

@herbert @mahesh I can do it now :)

@hdamker r1.1 released ✅

@mahesh you can now proceed with the PR for M4

@maheshc01 @Kevsy Would be good if you could correct the following within the CHANGELOG for r1.1 in course of the M4 release PR (versions should all have -rc.1 as appendix). There is always something which makes it through the reviews :-)

r1.1 - rc
Release Notes

This release contains the definition and documentation of
Connectivity Insights API v0.4.0
Connectivity Insights Subscriptions API v0.4.0
Application Profiles API v0.3.0

Within the GitHub release description I have already edited it.
Release tracker pages updated with the release tag r1.1